Skip to content

Conversation

@ysoldak
Copy link
Contributor

@ysoldak ysoldak commented Nov 9, 2025

This breaks dependency of ssd1xxx drivers from machine package, more specifically from machine.Pin.
See #795

To workaround the need to convert all High() and Low() calls to Set(true) and Set(false) respectively, I've opted for implementing a simple helper struct OutputStruct. A bit clunky, so I'm happy to apply a better approach if there is one. At least it's inside internal.

Psst, I've considered to use OutputFunc internally that has High() and Low() implemented already, but then hit the configure-for-output wall (need to maintain reference to the variable anyway and call the legacy method later).

Yes, by the way, legacy backwards-compatible pin configuration for baremetal targets moved to Configure methods, as New shall not touch hardware (at least we state this in many places and it is seem to be sane pattern). I don't expect this simple change shall break anyone.

Decided to make minimal changes and skip pin.OutputStruct

Breaking change though: returning pointer to struct from New() in ssd1289 case -- such changes we did before and it is very easy to fix on consumer side and it breaks with big bang on compile time, so shall be fine.

We are hopefully and gradually apply such changes to all drivers.
I propose we do this in batches of similar drivers to help with reviews.

@deadprogram
Copy link
Member

The following shows how to do the same thing by combining pin.Output and pin.OutputFunc for the ssd1306:

package ssd1306

import (
	"time"

	"tinygo.org/x/drivers"
	"tinygo.org/x/drivers/internal/legacy"
	"tinygo.org/x/drivers/internal/pin"
)

type SPIBus struct {
	wire     drivers.SPI
	dcPin    pin.OutputFunc
	resetPin pin.OutputFunc
	csPin    pin.OutputFunc
	buffer   []byte // buffer to avoid heap allocations
}

// NewSPI creates a new SSD1306 connection. The SPI wire must already be configured.
func NewSPI(bus drivers.SPI, dcPin, resetPin, csPin pin.Output) *Device {
	legacy.ConfigurePinOut(dcPin)
	legacy.ConfigurePinOut(resetPin)
	legacy.ConfigurePinOut(csPin)
	return &Device{
		bus: &SPIBus{
			wire:     bus,
			dcPin:    dcPin.Set,
			resetPin: resetPin.Set,
			csPin:    csPin.Set,
		},
	}
}

// configure pins with the SPI bus and allocate the buffer
func (b *SPIBus) configure(address uint16, size int16) []byte {
	b.csPin.Low()
	b.dcPin.Low()
	b.resetPin.Low()

	b.resetPin.High()
	time.Sleep(1 * time.Millisecond)
	b.resetPin.Low()
	time.Sleep(10 * time.Millisecond)
	b.resetPin.High()

	b.buffer = make([]byte, size+1) // +1 for a command
	return b.buffer[1:]             // return the image buffer
}

// command sends a command to the display
func (b *SPIBus) command(cmd uint8) error {
	b.buffer[0] = cmd
	return b.tx(b.buffer[:1], true)
}

// flush sends the image to the display
func (b *SPIBus) flush() error {
	return b.tx(b.buffer[1:], false)
}

// tx sends data to the display
func (b *SPIBus) tx(data []byte, isCommand bool) error {
	b.csPin.High()
	b.dcPin(!isCommand)
	b.csPin.Low()
	err := b.wire.Tx(data, nil)
	b.csPin.High()
	return err
}

Note that it does not need to add any additional types. 😄

@soypat
Copy link
Contributor

soypat commented Nov 10, 2025

+1 to proposed scheme by Ron. No reason to intentionally avoid all the benefits of pin.OutputFunc 😄

@ysoldak
Copy link
Contributor Author

ysoldak commented Nov 10, 2025

Yes, @deadprogram @soypat this going to work, but then we are configuring pins as outputs in NewSPI(), which is sort of a surprise behaviour, no?

I mean, it was like this before, but only by mistake -- pins configuration must have happened in Configure call, not in New. Means sort of contract we have: New() does not touch hardware, just makes an object.

Leaving aside the whole "configure pin mode in drivers was a big mistake from start".

I'm just trying to find a model approach to apply on majority of drivers breaking from machine. Configuring pin modes, even if legacy, for backwards compatibility, in New instead of Configure -- does not seem right? Or am I oversensitive?

@soypat
Copy link
Contributor

soypat commented Nov 10, 2025

#814 brings changes from #753 and shows many examples on how to defer configuration of pins to a configure call separate from the new constructor

@soypat
Copy link
Contributor

soypat commented Nov 10, 2025

To answer your question Yurii, yes it is surprise behaviour. I don't like configuration in New calls- but changing that behaviour may break users and we're already striving to not break our users with the proposed pin HAL API so we could try to not break anyone until we're ready to do a major announcement. For now lets focus on porting drivers to new pin HAL API like we're doing without stepping on too many toes- at least that's my take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants